Skip to content

fix(scheduler): prune printedLog when a node is deleted - #2939

Open
im-Toqeer-506 wants to merge 1 commit into
Project-HAMi:masterfrom
im-Toqeer-506:fix/printedlog-node-churn
Open

fix(scheduler): prune printedLog when a node is deleted#2939
im-Toqeer-506 wants to merge 1 commit into
Project-HAMi:masterfrom
im-Toqeer-506:fix/printedlog-node-churn

Conversation

@im-Toqeer-506

@im-Toqeer-506 im-Toqeer-506 commented Sep 1, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

printedLog was a loop-local map in RegisterFromNodeAnnotations, so onDelNode could not remove entries from it.

This caused two issues:

  • The map kept growing for the lifetime of the scheduler, with one entry for every node name seen.
  • If a node was deleted and recreated with the same name, it was logged as Node device updated at V(5) instead of Node device added at info level. This meant the replacement node was not visible in default-verbosity logs.

This PR moves printedLog into Scheduler state and protects it with the existing s.lock. Entries are removed in cleanupNodeUsage, which already handles per-node cleanup during onDelNode.

Which issue(s) this PR fixes:

Fixes #2937

Validation:

  • Added Test_register_PrintedLogPrunedOnNodeDelete covering add → delete → re-add and ensuring other nodes remain in the map.
  • Test_register_NodeCacheConcurrency already tests concurrent register() and onDelNode() calls. It passes with -race.
  • Full suite: -short --race -count=1 — 38 packages pass.
  • golangci-lint v2.13.1 — 0 issues.
  • gofmt, goimports, and license checks pass.
  • TestPrepareHostPIDLockParent still fails in my environment, but the same failure occurs on a clean upstream/master, so it is pre-existing and unrelated to this PR.

Does this PR introduce a user-facing change?

NONE

AI assistance disclosure: AI was used to assist with the analysis, implementation, and tests. I reviewed the changes and verified the results.

Summary by CodeRabbit

  • Bug Fixes
    • Improved scheduler logging when nodes are removed and later recreated.
    • Ensured recreated nodes are recognized as new and their device information is logged again.
    • Preserved logging state for nodes that remain available.
  • Tests
    • Added coverage for node deletion, recreation, and concurrent registration scenarios.

@im-Toqeer-506
fix(scheduler): prune printedLog when a node is deleted
03deb48 
Signed-off-by: M Toqeer Zia <muhammadtoqeerzia586694@gmail.com>
@hami-robot hami-robot Bot added the kind/bug Something isn't working label Sep 1, 2026
@hami-robot

hami-robot Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: im-Toqeer-506
Once this PR has been reviewed and has the lgtm label, please assign shouren for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/scheduler/scheduler.go 80.00% 1 Missing ⚠️
Flag Coverage Δ
unittests 70.35% <80.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/scheduler/scheduler.go 77.10% <80.00%> (+0.30%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai

coderabbitai Bot commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: ddd7cd27-973b-43c8-94df-a95a175f104a

📥 Commits

Reviewing files that changed from the base of the PR and between e6932f5 and 03deb48.

📒 Files selected for processing (3)
  • pkg/scheduler/register_race_test.go
  • pkg/scheduler/scheduler.go
  • pkg/scheduler/scheduler_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The scheduler now owns the node printed-log map, removes entries during node cleanup, and uses a single-argument register method. Tests cover registration callers and node deletion followed by same-name recreation.

Changes

Printed log lifecycle

Layer / File(s) Summary
Scheduler state and cleanup
pkg/scheduler/scheduler.go		 Scheduler initializes and uses printedLog. cleanupNodeUsage removes deleted node entries. register no longer receives the map as an argument.
Registration call sites and lifecycle coverage
pkg/scheduler/register_race_test.go, pkg/scheduler/scheduler_test.go		 Tests use the updated register signature. Lifecycle coverage verifies deletion removes only the deleted node and allows same-name recreation to be logged as new.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 03deb

This change prunes deleted-node log state and restores correct logging when a node is recreated; no actionable merge-blocking risk remains.

Suggested reviewers: mesutoezdil, wangmin362

Poem

A rabbit checks the node log bright
Deleted names leave sight
New nodes hop into the queue
Old entries clear as they should do
The scheduler keeps its record right

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the scheduler fix: pruning printedLog when a node is deleted.
Linked Issues check ✅ Passed The changes satisfy issue #2937. printedLog is stored on Scheduler, initialized in NewScheduler, removed from register parameters, pruned during cleanupNodeUsage, and covered by add/delete/re-add test…
Out of Scope Changes check ✅ Passed All changes support issue #2937. The register_race_test.go update is a necessary caller adjustment for the register signature change, not unrelated work.
Full details: Linked Issues check

Explanation

The changes satisfy issue #2937. printedLog is stored on Scheduler, initialized in NewScheduler, removed from register parameters, pruned during cleanupNodeUsage, and covered by add/delete/re-add tests.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai
coderabbitai Bot requested a review from Wangmin362 September 1, 2026 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chaunceyjiang chaunceyjiang Awaiting requested review from chaunceyjiang
@mesutoezdil mesutoezdil Awaiting requested review from mesutoezdil
@Wangmin362 Wangmin362 Awaiting requested review from Wangmin362

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cheduler: printedLog map is never pruned when nodes are deleted

1 participant

@im-Toqeer-506